There's a few issues I still need to resolve:

1. This breaks some test cases in StrncpyFlippedArgs, which I think are a result of strlen being assumed to have side effects in IR generation. I'll probably fix that in a separate PR, since it should be an uncontroversial quick fix.
2. The GVN class extends ValueNumber, which currently reports an IR-based toString. We could change ValueNumber's toString, make GVN a newtype with an AST-based toString, or move the wrapper into the IR library and have GVN extend TValueNumber directly.
3. I didn't reimplement the getKind() method, which is used in a test. I'm inclined to add it to the IR's ValueNumber class.

There's also a usability question. Should GVN::getAnExpr be implemented with getAnInstruction().getConvertedResultExpression() or getAnInstruction().getAST()? Using getAST means every GVN generated from an Expr has a link to the Expr, but might produce confusing results...

For 2, I think it's ValueNumber.toString and ValueNumber.getLocation that that need to change. These are currently implemented in terms of getExampleInstruction, but getExampleInstruction is an expensive operation that uses Instruction.getUniqueId, whose QLDoc says "This is used for sorting IR output for tests, and is likely to be inefficient for any other use." We don't want to invoke such expensive operations implicitly. Is it important that the AST-based GVN and the AST-wrapped IR GVN produce the same toString? Would it be a problem to make it just string toString() { result = "ValueNumber" }?

For 4, I'd expect us to use getUnconvertedResultExpression. That's un-converted because AFAICT the AST-based GVN library ignores conversions.

#1566 is the PR handling strlen's side effects.

Is it important that the AST-based GVN and the AST-wrapped IR GVN produce the same toString? Would it be a problem to make it just string toString() { result = "ValueNumber" }?

I think it is important that the AST-wrapped IR GVN has an AST-based toString and getLocation for debuggability by users who aren't familiar with the IR, but that doesn't need to be identical to what the AST-based GVN would report.

For 4, I'd expect us to use getUnconvertedResultExpression. That's un-converted because AFAICT the AST-based GVN library ignores conversions.

I think it does value-number conversions separately, but given the usual pattern of ignoring conversions in the AST, it will often be used on unconverted values.

I think it is important that the AST-wrapped IR GVN has an AST-based toString and getLocation for debuggability by users who aren't familiar with the IR, but that doesn't need to be identical to what the AST-based GVN would report.

How about something like result = "ValueNumber: " + strictconcat(this.getAnInstruction().toString(), ", ")? That should be helpful for debugging and not overly expensive. Even if a value number is used in 1000 places, the concatenated string should be short as long as the string representation of most of those places is Load: x.

For getLocation(), you could take the Location that's lexicographically smallest according to hasLocationInfo. To avoid any ambiguity, you could also avoid implementing getLocation() on ValueNumber and just implement hasLocationInfo().

For this wrapper, I'd prefer a toString that doesn't mention instruction names at all. Would result = "ValueNumber: " + strictconcat(this.getAnInstruction().getUnconvertedResultExpression().toString(), ", ") work?

I'm happy with the proposed getLocation.

This is what I think we agreed on yesterday in the meeting:

1. toString can be just result = "GVN". Separately, there can be a debugging mechanism that does the strictconcat and is allowed to expose implementation details.
2. We should be able to put two different toString predicates on the same newtype, but it will require rearranging the code. The newtype will have to be public in a file that's placed in an internal subdir. (We can't put the two toStrings in the same file because then the IR GVN won't be language-independent).
3. This PR should be merged around the same time as we put the IR security.TaintTracking wrapper into production.

superseded by #2765